Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve syntax highlighting colors for dark theme in MANIFEST.MF #1504

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vogella
Copy link
Contributor

@vogella vogella commented Dec 4, 2024

Changing the color to the same as for classes in the Java editor and removing the bold setting.

Fixes #1503

@vogella
Copy link
Contributor Author

vogella commented Dec 4, 2024

cc @BeckerWdf and @mvm-sap WDYT?

Copy link

github-actions bot commented Dec 4, 2024

Test Results

12 files   -    273  12 suites   - 273   6s ⏱️ - 50m 26s
26 tests  -  3 560  26 ✅  -  3 484  0 💤  -  76  0 ❌ ±0 
48 runs   - 10 902  48 ✅  - 10 671  0 💤  - 231  0 ❌ ±0 

Results for commit 1372671. ± Comparison against base commit b8ce236.

This pull request removes 3560 tests.
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_importPackage
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_includeFragments
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_includeFragmentsProvidingPackages
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_includeNonTestFragments
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_includeOptional
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_requireBundle
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_requireBundle2
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_requireDifferentVersions
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_requiredCapability
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.WorkspaceModelManagerTest ‑ testBundleRootHandling_bundleRootChangedFromDefaultToOthersAndReverse
…

♻️ This comment has been updated with latest results.

@vogella vogella force-pushed the manifest-syntax-highlighting branch from c2c3e3a to c368ad8 Compare December 4, 2024 07:21
@mvm-sap
Copy link

mvm-sap commented Dec 4, 2024

cc @BeckerWdf and @mvm-sap WDYT?

Yes, keywords look sharper and clearer now than in the old ones. Looks good to me.

Changing the color to the same as for classes in the Java editor and
removing the bold setting.

Fixes eclipse-pde#1503
@vogella vogella force-pushed the manifest-syntax-highlighting branch from c368ad8 to 1372671 Compare December 6, 2024 11:00
@vogella
Copy link
Contributor Author

vogella commented Dec 6, 2024

@BeckerWdf you made a comment in the forked repo. Please add it here.

@BeckerWdf
Copy link
Contributor

@BeckerWdf you made a comment in the forked repo. Please add it here.

Yes. I noticed it also. See comments here.

'editor.color.header_key=221,40,103'
'editor.color.header_key_bold=true'
'editor.color.header_osgi=38,139,210'
'editor.color.header_key=18,144,195'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you are changing from red-ish to blue-ish?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BeckerWdf Tbh I changed it during testing and later I forgot about it as I did not find a key in the manifest which uses it. Are you aware of such a key? I can change it back for this key if desired just to be save.

@laeubi
Copy link
Contributor

laeubi commented Dec 14, 2024

@vogella for style changes please always give before/after screenshot and provide dark/light mode examples if changing especially for darkmode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve syntax highlighting colors for dark theme in MANIFEST.MF
4 participants